Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from Yarn to pnpm #46

Merged
merged 14 commits into from
Nov 28, 2023
Merged

Migrate from Yarn to pnpm #46

merged 14 commits into from
Nov 28, 2023

Conversation

nkuba
Copy link
Member

@nkuba nkuba commented Nov 23, 2023

Use pnpm as a package manager

pnpm is a package manager optimized for monorepos.
It is faster than NPM and consumes less disk storage than Yarn.

Disk consumption:

  • Yarn
$ du -sh ~/workspace/acre/acre
2.2G    /Users/jakub/workspace/acre/acre
  • pnpm
$  du -sh ~/workspace/acre/acre
875M    /Users/jakub/workspace/acre/acre

To install pnpm with Homebrew run brew install pnpm, for other installation options
please see the documentation.

To install the packages dependencies run:

pnpm install

@nkuba nkuba changed the title Pnpm Migrate from Yarn to pnpm Nov 23, 2023
@nkuba nkuba marked this pull request as ready for review November 23, 2023 14:49
[pnpm](https://github.com/pnpm/pnpm) is a package manager optimized for monorepos.
It is faster than NPM and consumes less disk storage than Yarn.

Disk consumption:
- Yarn
```sh
$ du -sh ~/workspace/acre/acre
2.2G    /Users/jakub/workspace/acre/acre
```
- pnpM
```sh
$  du -sh ~/workspace/acre/acre
875M    /Users/jakub/workspace/acre/acre
```

To install pnpm with Homebrew run `brew install pnpm`, for other installation options
please see the [documentation](https://pnpm.io/installation).

To install the packages dependencies run:
```sh
pnpm install
```
Use pnpm instead of yarn in CI workflow.

Define `packageManager` in `package.json` so the `pnpm/action-setup@v2`
job uses it to setup the correct pnpm version.
Gatsby requires a plugin to work with pnpm.
Be explicit in the command that we want to run a script and pass the
paratemeters to the script
Instead of adding dependencies we define public-hoist-pattern property
for @chakra-ui/* packages.

Read more on public-hoist-pattern here: https://pnpm.io/npmrc#public-hoist-pattern

The fix was suggested in chakra-ui/chakra-ui#6427 (comment)
Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also update the README.md files for the website and dApp. We should now use pnpm install and pnpm start.

@dimpar
Copy link
Member

dimpar commented Nov 27, 2023

Can we get rid of yarn-debug.log* and yarn-error.log* from .gitignore files in both website and dApp dirs?

@dimpar dimpar mentioned this pull request Nov 27, 2023
Define main .gitignore in the root directory with a common rules and
separate .gitignore files with module-specific rules.
dApp and website docs were missing pnpm commands.
@nkuba nkuba requested a review from kkosiorowska November 27, 2023 21:13
r-czajkowski
r-czajkowski previously approved these changes Nov 28, 2023
Copy link
Contributor

@r-czajkowski r-czajkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's resolve conflicts. Other than that LGTM 🚀

Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments:

  • I wonder what is the correct approach for pnpm. Should the packages for each repository be installed separately? From the main app root, we can install all packages using the pnpm install. In the case of the website, everything works correctly when packages are installed from the root. For dApp there is a problem with @chakra-ui/theme-tools. I would like to clear up any doubts about the way packages are installed.

Screenshot 2023-11-28 at 09 10 58

  • If it is possible to install packages from root level then I would suggest adding the .nvmrc file.

Screenshot 2023-11-28 at 08 59 47

  • This thread has already been discussed here. I wonder if we need the .nmprc file.

The root directory contains package.json, so it is worth adding the
.nvmrc file for consistency.
When npmrc was defined with public hoist in the `dapp` directory it
required installation of the dependencies to happen inside the `dapp`
directory to correctly resolve the `@chakra-ui/theme-tools` dependency.

With moving the file to the root directory, installation of the
dependencies inside the root directory will correctly resolve
`@chakra-ui` dependencies inside the `dapp` sub-project.
@nkuba
Copy link
Member Author

nkuba commented Nov 28, 2023

@kkosiorowska thank you for the comments!

  • I wonder what is the correct approach for pnpm. Should the packages for each repository be installed separately? From the main app root, we can install all packages using the pnpm install. In the case of the website, everything works correctly when packages are installed from the root. For dApp there is a problem with @chakra-ui/theme-tools. I would like to clear up any doubts about the way packages are installed.

Fixed in 08a3668

  • If it is possible to install packages from root level then I would suggest adding the .nvmrc file.

Added in d34b104

  • This thread has already been discussed here. I wonder if we need the .nmprc file.

Please check with the latest changes. It worked fine on my machine:

*[pnpm][~/workspace/acre/acre/dapp]$ pnpm lint:js

> [email protected] lint:js /Users/jakub/workspace/acre/acre/dapp
> eslint .

=============

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=4.3.5 <5.3.0

YOUR TYPESCRIPT VERSION: 5.3.2

Please only submit bug reports when using the officially supported version.

=============

The job has to be updated to use pnpm instead of yarn.
Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments have been implemented. Ready for merge 🚀

We need .npmrc to properly install the dependencies. However, this change doesn't solve the problem with import/no-extraneous-dependencies. Let's leave the rule as it is for now.

@kkosiorowska kkosiorowska merged commit ff60a53 into main Nov 28, 2023
9 checks passed
@kkosiorowska kkosiorowska deleted the pnpm branch November 28, 2023 15:26
r-czajkowski added a commit that referenced this pull request Nov 30, 2023
Depends on #46

### Syncpack
[Syncpack](https://jamiemason.github.io/syncpack/) is a tool that helps
manage
multiple package.json files in a monorepo.
#### Install
In the repository's root directory execute:
```sh
pnpm install
```
#### Usage
To list dependencies from all packages run:
```sh
pnpm syncpack list
``` 
To update a dependency (e.g. `eslint`) in all packages run:
```sh
pnpm syncpack update --filter eslint
```

### Example

![image](https://github.com/thesis/acre/assets/10741774/a948865e-c10d-464e-822d-f9d734d1dfb8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ Backend APIs, Maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants